feat: implement Multi-Provider with parity fixes#579
feat: implement Multi-Provider with parity fixes#579jonathannorris wants to merge 8 commits intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement by adding a Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #579 +/- ##
==========================================
- Coverage 98.31% 95.13% -3.19%
==========================================
Files 43 45 +2
Lines 2075 2815 +740
==========================================
+ Hits 2040 2678 +638
- Misses 35 137 +102
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a MultiProvider feature, allowing OpenFeature clients to manage and evaluate multiple feature flag providers using various strategies (FirstMatch, FirstSuccessful, Comparison) with both sequential and parallel execution modes. It includes a new InternalHookProvider protocol to enable providers to manage their own hook execution and status internally, which the MultiProvider implements. The client-side flag evaluation logic has been updated to integrate this new internal hook management. Review comments suggest improving the readability of provider_hooks assignment, adding a comment to clarify ContextVars propagation in ThreadPoolExecutor for older Python versions, and enhancing the docstring for ComparisonStrategy.determine_final_result to better explain its behavior.
| provider_hooks = ( | ||
| [] | ||
| if self._provider_uses_internal_hooks(provider) | ||
| else provider.get_provider_hooks() | ||
| ) |
There was a problem hiding this comment.
The provider_hooks variable is assigned an empty list or the result of provider.get_provider_hooks(). It would be more readable to assign the result of provider.get_provider_hooks() directly to provider_hooks and then conditionally clear the list if _provider_uses_internal_hooks returns true. This simplifies the logic and makes it easier to understand the intent.
provider_hooks = provider.get_provider_hooks()
if self._provider_uses_internal_hooks(provider):
provider_hooks = []| # Each worker thread gets its own copy of the current context so | ||
| # that ContextVars (e.g. _hook_runtime) are propagated correctly. | ||
| # ThreadPoolExecutor does not automatically copy context on | ||
| # Python < 3.12, and a single Context.run() is not reentrant. |
There was a problem hiding this comment.
Each worker thread gets its own copy of the current context so that ContextVars (e.g. _hook_runtime) are propagated correctly. ThreadPoolExecutor does not automatically copy context on Python < 3.12, and a single Context.run() is not reentrant. Consider adding a comment that this is a workaround for Python versions < 3.12.
| # The first provider's result is the "final resolution" (used on agreement). | ||
| # The fallback provider's result is used on mismatch (per JS SDK reference). | ||
| final_evaluation = evaluations[0] | ||
| fallback_evaluation = self._select_fallback_evaluation(evaluations) | ||
| reference_value = final_evaluation.result.value | ||
| has_mismatch = any( | ||
| evaluation.result.value != reference_value for evaluation in evaluations | ||
| ) | ||
| if has_mismatch: | ||
| if self.on_mismatch is not None: | ||
| mismatch_results = { | ||
| evaluation.provider_name: evaluation.result | ||
| for evaluation in evaluations | ||
| } | ||
| try: | ||
| self.on_mismatch(flag_key, mismatch_results) | ||
| except Exception: | ||
| logger.exception( | ||
| "Comparison strategy mismatch callback failed for flag '%s'", | ||
| flag_key, | ||
| ) | ||
| return fallback_evaluation.result | ||
| return final_evaluation.result |
There was a problem hiding this comment.
In the ComparisonStrategy.determine_final_result method, the first provider's result is used as the "final resolution" when there is agreement. However, the fallback provider's result is used on mismatch. This behavior should be clearly documented in the method's docstring to ensure that users understand how the final result is determined.
Implements the Multi-Provider as specified in OpenFeature Appendix A.
The Multi-Provider wraps multiple underlying providers in a unified interface,
allowing a single client to interact with multiple flag sources simultaneously.
Key features implemented:
- MultiProvider class extending AbstractProvider
- FirstMatchStrategy (sequential evaluation, stops at first success)
- EvaluationStrategy protocol for custom strategies
- Provider name uniqueness (explicit, metadata-based, or auto-indexed)
- Parallel initialization of all providers with error aggregation
- Support for all flag types (boolean, string, integer, float, object)
- Hook aggregation from all providers
Use cases:
- Migration: Run old and new providers in parallel
- Multiple data sources: Combine env vars, files, and SaaS providers
- Fallback: Primary provider with backup sources
Example usage:
provider_a = SomeProvider()
provider_b = AnotherProvider()
multi = MultiProvider([
ProviderEntry(provider_a, name="primary"),
ProviderEntry(provider_b, name="fallback")
])
api.set_provider(multi)
Closes #511
Signed-off-by: vikasrao23 <vikasrao23@users.noreply.github.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
…hancements Address Gemini code review feedback: - Update initialize() docstring to reflect sequential (not parallel) initialization - Add documentation notes to all async methods explaining they currently delegate to sync - Clarify that parallel evaluation mode is planned but not yet implemented - Update EvaluationStrategy protocol docs to set correct expectations This brings documentation in line with actual implementation. True async and parallel execution will be added in follow-up PRs. Refs: #511 Signed-off-by: vikasrao23 <vikasrao23@users.noreply.github.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
CRITICAL FIXES: - Fix FlagResolutionDetails initialization - remove invalid flag_key parameter - Add error_code (ErrorCode.GENERAL) to all error results per spec HIGH PRIORITY: - Implement true async evaluation using _evaluate_with_providers_async - All async methods now properly await provider async methods (no blocking) - Implement parallel provider initialization using ThreadPoolExecutor IMPROVEMENTS: - Remove unused imports (asyncio, ProviderEvent, ProviderEventDetails, ProviderStatus) - Add ErrorCode import for proper error handling - Cache provider hooks to avoid re-aggregating on every evaluation - Update docstrings to clarify current implementation status Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
HIGH PRIORITY FIXES: - Fix name resolution logic to prevent collisions between explicit and auto-generated names - Check used_names set for metadata names before using them - Use while loop to find next available indexed name if collision detected - Implement event propagation (spec requirement) - Override attach() and detach() methods to forward events to all providers - Import ProviderEvent and ProviderEventDetails - Enables cache invalidation and other event-driven features MEDIUM PRIORITY IMPROVEMENTS: - Parallel shutdown with proper error logging - Use ThreadPoolExecutor for concurrent shutdown - Add logging for shutdown failures - Optimize ThreadPoolExecutor max_workers - Set to len(providers) for both initialize() and shutdown() - Ensures all providers can start immediately - Improve type hints for better type safety - Add generic type parameters to FlagResolutionDetails in resolve_fn signatures - Specify Awaitable return type for async resolve_fn - Add generic types to results list declarations All critical and high-priority feedback addressed. Ready for re-review. Refs: #511 Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
This is more consistent with the other type imports in the file. Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Co-authored-by: jonathan <jonathan@taplytics.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
- Fix ContextVar propagation to ThreadPoolExecutor workers (Python <3.12) - Fix _refresh_aggregate_status dropping events during partial init failure - Add shouldEvaluateThisProvider check to skip NOT_READY/FATAL providers - Fix ComparisonStrategy to return first provider result on no-mismatch - Add InternalHookProvider protocol replacing fragile duck-typing - Scope get_status override in registry to InternalHookProvider only - Rename camelCase instance variables to snake_case Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
- Add _is_internal_hook_provider class marker to avoid Mock false positives with runtime_checkable Protocol isinstance checks - Fix mypy no-redef errors by hoisting evaluations declaration before branch - Fix mypy no-any-return by assigning to typed local before returning - Fix mypy attr-defined by using _as_internal_hook_provider narrowing helper - Apply ruff formatting fixes Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
jonathannorris
force-pushed
the
feat/multi-provider-parity-568
branch
from
e1145f6 to
d6fda15
Compare
4 participants
Summary
MultiProviderwithFirstMatchStrategy,FirstSuccessfulStrategy, andComparisonStrategyInternalHookProviderprotocolParity Fixes
Addresses gaps identified in cross-SDK comparison against the js-sdk reference (#568):
contextvars.copy_context()so_hook_runtimeis propagated correctly on Python < 3.12_refresh_aggregate_statusnow acceptsforce=Trueduringinitialize()to ensure events are emitted even when aggregate status staysNOT_READY_should_evaluate_providerto skipNOT_READY/FATALproviders during evaluation, matchingshouldEvaluateThisProviderin the JS SDKdetermineFinalResultbehaviorgetattr/callableduck-typing with a@runtime_checkableprotocol in bothclient.pyand_registry.pyget_provider_statusnow checksisinstance(provider, InternalHookProvider)instead of genericgetattr(provider, "get_status"), preventing accidental status override by unrelated providersRelated Issues
Fixes #568